Skip to content

Add read_buf equivalents for positioned reads #140459

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add read_buf equivalents for positioned reads #140459

wants to merge 1 commit into from
+194 −21

Conversation

niklasf
Copy link
Contributor

@niklasf niklasf commented Apr 29, 2025 •
edited by rustbot
Loading

Adds the following items under the read_buf (#78485) read_buf_at (#140771) feature:

  • std::os::unix::fs::FileExt::read_buf_at
  • std::os::unix::fs::FileExt::read_buf_exact_at
  • std::os::windows::fs::FileExt::seek_read_buf

try-job: x86_64-msvc-1
try-job: x86_64-msvc-2

@rustbot
Copy link
Collaborator

rustbot commented Apr 29, 2025

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-unix Operating system: Unix-like O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 29, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@niklasf niklasf closed this Apr 29, 2025
@niklasf niklasf force-pushed the feature/read-buf-at branch from 74e016c to 2ccb45f Compare April 29, 2025 14:07
@niklasf niklasf reopened this Apr 29, 2025
@tgross35
Copy link
Contributor

tgross35 commented May 1, 2025

Have these changes been discussed with libs-api at all? Usually changes to unstable API need to be proposed at https://github.com/rust-lang/libs-team/ with the ACP issue template.

This needs some tests as well.

@tgross35
Copy link
Contributor

tgross35 commented May 1, 2025

For the above,
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 1, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 1, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@niklasf niklasf mentioned this pull request May 7, 2025
Open
4 tasks
@Kivooeo
Copy link
Contributor

Kivooeo commented May 8, 2025

For consistency and clarity, could we include safety comments on each unsafe block? Even brief notes are helpful to understand the assumptions being made

@niklasf niklasf force-pushed the feature/read-buf-at branch from 2ccb45f to 09ac65a Compare May 12, 2025 21:45
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@niklasf niklasf force-pushed the feature/read-buf-at branch from 09ac65a to a22d2b4 Compare May 12, 2025 22:06
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@niklasf niklasf force-pushed the feature/read-buf-at branch from a22d2b4 to 59cb58e Compare May 12, 2025 22:30
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@niklasf
Add read_buf equivalents for positioned reads
85e1da1 
Adds the following items under the `read_buf_at` (rust-lang#140771) feature:

 - `std::os::unix::FileExt::read_buf_at`
 - `std::os::unix::FileExt::read_buf_exact_at`
 - `std::os::windows::FileExt::seek_read_buf`
@niklasf niklasf force-pushed the feature/read-buf-at branch from 59cb58e to 85e1da1 Compare May 12, 2025 22:41
@niklasf
Copy link
Contributor Author

niklasf commented May 12, 2025 •
edited
Loading

Thank you both.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 12, 2025
tgross35
tgross35 reviewed May 28, 2025
View reviewed changes
library/std/src/sys/fd/unix.rs
}

pub fn read_buf(&self, mut cursor: BorrowedCursor<'_>) -> io::Result<()> {
// Safety: `cursor.as_mut()` starts with `cursor.capacity()` writable bytes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: // SAFETY: comments are uppercase

library/std/src/os/unix/fs.rs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The newly added API needs some doc examples

library/std/src/sys/fd/unix.rs
Comment on lines +90 to +101
#[cfg(not(any(
all(target_os = "linux", not(target_env = "musl")),
target_os = "android",
target_os = "hurd"
)))]
use libc::pread as pread64;
#[cfg(any(
all(target_os = "linux", not(target_env = "musl")),
target_os = "android",
target_os = "hurd"
))]
use libc::pread64;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is getting moved anyway, mind simplifying this with cfg_select!?

@tgross35
Copy link
Contributor

@bors try

bors added a commit that referenced this pull request May 28, 2025
@bors
Auto merge of #140459 - niklasf:feature/read-buf-at, r=<try>
f769285 
Add `read_buf` equivalents for positioned reads

Adds the following items under the ~~`read_buf` (#78485)~~ `read_buf_at` (#140771) feature:

 - `std::os::unix::fs::FileExt::read_buf_at`
 - `std::os::unix::fs::FileExt::read_buf_exact_at`
 - `std::os::windows::fs::FileExt::seek_read_buf`

try-job: x86_64-msvc-1
try-job: x86_64-msvc-2
@bors
Copy link
Collaborator

bors commented May 28, 2025

⌛ Trying commit 85e1da1 with merge f769285...

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-msvc-1 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot) 
Updating files:  98% (52406/53474)
Updating files:  99% (52940/53474)
Updating files: 100% (53474/53474)
Updating files: 100% (53474/53474), done.
branch 'try' set up to track 'origin/try'.
Switched to a new branch 'try'
##[endgroup]
[command]"C:\Program Files\Git\bin\git.exe" log -1 --format=%H
f76928546c97438a758c27b2f9381b78f73fcbc0
##[group]Run src/ci/scripts/setup-environment.sh
src/ci/scripts/setup-environment.sh
---
file:.git/config remote.origin.url=https://github.com/rust-lang/rust
file:.git/config remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*
file:.git/config gc.auto=0
file:.git/config http.https://github.com/.extraheader=AUTHORIZATION: basic ***
file:.git/config branch.try.remote=origin
file:.git/config branch.try.merge=refs/heads/try
file:.git/config submodule.library/backtrace.active=true
file:.git/config submodule.library/backtrace.url=https://github.com/rust-lang/backtrace-rs.git
file:.git/config submodule.library/stdarch.active=true
file:.git/config submodule.library/stdarch.url=https://github.com/rust-lang/stdarch.git
file:.git/config submodule.src/doc/book.active=true
---
failures:

---- fs::tests::test_seek_read_buf stdout ----

thread 'fs::tests::test_seek_read_buf' panicked at library\std\src\fs\tests.rs:635:9:
assertion `left == right` failed
  left: 9
 right: 0


failures:
    fs::tests::test_seek_read_buf

test result: FAILED. 561 passed; 1 failed; 5 ignored; 0 measured; 0 filtered out; finished in 56.85s

error: test failed, to rerun pass `-p std --lib`
Build completed unsuccessfully in 1:47:18
make: *** [Makefile:113: ci-msvc-py] Error 1
  local time: Thu May 29 01:02:38 CUT 2025
  network time: Thu, 29 May 2025 01:02:38 GMT
##[error]Process completed with exit code 2.
Post job cleanup.
[command]"C:\Program Files\Git\bin\git.exe" version

@bors
Copy link
Collaborator

bors commented May 29, 2025

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgross35 tgross35 tgross35 left review comments

Assignees

@tgross35 tgross35

Labels
O-unix Operating system: Unix-like O-windows Operating system: Windows S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@niklasf @rustbot @rust-log-analyzer @tgross35 @Kivooeo @bors